feat(applicationsignals-alpha): introduce Application Signals L2 constructs #32931

bjrara · 2025-01-15T01:51:53Z

Issue # (if applicable)

N/A

Description of changes

This PR adds L2 constructs to simplify the Application Signals enablement process. See aws/aws-cdk-rfcs#672 for more details.

Describe any new or updated permissions being added

Checklist

My code adheres to the CONTRIBUTING GUIDE and DESIGN GUIDELINES

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license

aws-cdk-automation

The pull request linter has failed. See the aws-cdk-automation comment below for failure reasons. If you believe this pull request should receive an exemption, please comment and provide a justification.

A comment requesting an exemption should contain the text Exemption Request. Additionally, if clarification is needed add Clarification Request to a comment.

✅ Updated pull request passes all PRLinter validations. Dismissing previous PRLinter review.

codecov · 2025-01-15T02:10:33Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 82.38%. Comparing base (e6f5bc8) to head (29d9629).

Additional details and impacted files

@@           Coverage Diff           @@
##             main   #32931   +/-   ##
=======================================
  Coverage   82.38%   82.38%           
=======================================
  Files         120      120           
  Lines        6937     6937           
  Branches     1170     1170           
=======================================
  Hits         5715     5715           
  Misses       1119     1119           
  Partials      103      103

Flag	Coverage Δ
suite.unit	`82.38% <ø> (ø)`

Flags with carried forward coverage won't be shown. Click here to find out more.

Components	Coverage Δ
packages/aws-cdk	`∅ <ø> (∅)`
packages/aws-cdk-lib/core	`82.38% <ø> (ø)`

🚀 New features to boost your workflow:

❄ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

moelasmar · 2025-01-16T06:17:44Z

@Mergifyio refresh

mergify · 2025-01-16T06:17:49Z

refresh

✅ Pull request refreshed

packages/@aws-cdk/aws-applicationsignals-alpha/.eslintrc.js

packages/@aws-cdk/aws-applicationsignals-alpha/package.json

packages/@aws-cdk/aws-applicationsignals-alpha/README.md

moelasmar · 2025-01-21T03:44:19Z

packages/@aws-cdk/aws-applicationsignals-alpha/lib/enablement/ecs.ts

+      throw new Error('Fargate tasks must deploy CloudWatch Agent as a sidecar container');
+    }
+    if (cloudWatchAgentConfig.enableSidecar) {
+      props.taskDefinition.taskRole.addManagedPolicy(ManagedPolicy.fromAwsManagedPolicyName('CloudWatchAgentServerPolicy'));


using this Managed policy violates the Apply least-privilege permissions rule of the security best practices, since as defined here, this managed policy enable multiple actions for * resources. It will be petter to add a new Policy, and add the specific resources that you need these actions to be allowed on them.

packages/@aws-cdk/aws-applicationsignals-alpha/lib/enablement/ecs.ts

packages/@aws-cdk/aws-applicationsignals-alpha/package.json

Pull request has been modified.

aws-cdk-automation · 2025-02-13T00:11:29Z

This PR has been in the BUILD FAILING state for 3 weeks, and looks abandoned. Note that PRs with failing linting check or builds are not reviewed, please ensure your build is passing

To prevent automatic closure:

Resume work on the PR
OR request an exemption by adding a comment containing 'Exemption Request' with justification e.x "Exemption Request: "
OR request clarification by adding a comment containing 'Clarification Request' with a question e.x "Clarification Request: "

This PR will automatically close in 7 days if no action is taken.

aws-cdk-automation · 2025-02-21T00:11:15Z

This PR has been deemed to be abandoned, and will be automatically closed. Please create a new PR for these changes if you think this decision has been made in error.

github-actions · 2025-02-21T00:11:38Z

Comments on closed issues and PRs are hard for our team to see.
If you need help, please open a new issue that references this one.

moelasmar · 2025-02-23T00:10:05Z

@bjrara .. are you planning to finalize this PR ?

bjrara · 2025-02-24T18:46:56Z

@bjrara .. are you planning to finalize this PR ?

Yes.

bjrara · 2025-03-13T06:34:47Z

All the major comments are addressed. Only two comments to add additional document link for used environment variables are left open.

aws-cdk-automation · 2025-03-18T07:07:58Z

AWS CodeBuild CI Report

CodeBuild project: AutoBuildv2Project1C6BFA3F-wQm2hXv2jqQv
Commit ID: 29d9629
Result: FAILED
Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

moelasmar

thanks @bjrara for addressing my previous comments. Code looks very good, I just left some comments related to testing.

moelasmar · 2025-03-19T16:40:45Z

packages/@aws-cdk/aws-applicationsignals-alpha/README.md

+#### Enable Application Signals on ECS with sidecar mode
+
+1. Configure `instrumentation` to instrument the application with the ADOT Java Agent.
+2. Setting `enableSidecar` to true to add the CloudWatch Agent as a sidecar container.


i think this step is not valid any more, at least I could not find it in the below example.

moelasmar · 2025-03-19T16:47:17Z

packages/@aws-cdk/aws-applicationsignals-alpha/README.md

+1. Run CloudWatch Agent as a daemon service with service connect.
+1. Configure `instrumentation` to instrument the application with the ADOT Python Agent.
+1. Set `enableSidecar` to false to disable running CloudWatch agent as a sidecar.
+1. Override environment variables by configuring `overrideEnvironments` to use service connect endpoints to communicate to the CloudWatch agent server


I believe these steps do not match the below example. Could you please update them.

moelasmar · 2025-03-19T17:13:24Z

packages/@aws-cdk/aws-applicationsignals-alpha/test/ecs-enablement.test.ts

can you refactor the unit test cases to split them in files that match your new files

moelasmar · 2025-03-19T17:37:37Z

packages/@aws-cdk/aws-applicationsignals-alpha/lib/enablement/ecs.ts

+        props.instrumentation.sdkVersion,
+        overrideEnvironments,
+      );
+    }


can you add else here to throw error that this is not supported instrumentation

moelasmar · 2025-03-19T18:46:03Z

packages/@aws-cdk/aws-applicationsignals-alpha/test/integ.ecs-enablement-daemon.ts

Can you cover all the supported instrumentation, since we do different configuration for each language. Also, can you add assertion logic, to verify 1\The ECS tasks are still running successfully after we applied the application signals changes 2\ If it is possible, to check if we can find monitoring data got pushed to Application signals in Cloud watch.

moelasmar · 2025-03-19T18:47:42Z

packages/@aws-cdk/aws-applicationsignals-alpha/test/integ.ecs-enablement-sidecar.ts

similar to the previous test case, increase the coverage of instrumentations.
Also, I think Sidecar, can also be applied to EC2 Ecs tasks, so can you add another integration test case that cover that.

moelasmar · 2025-03-19T19:03:20Z

packages/@aws-cdk/aws-applicationsignals-alpha/test/integ.ecs-enablement-daemon.ts

+      value: 'http://cwagent-4316-http:4316/v1/metrics',
+    }, {
+      name: appsignals.TraceExporting.OTEL_EXPORTER_OTLP_TRACES_ENDPOINT,
+      value: 'hhttp://cwagent-4316-http:4316/v1/traces',


typo in hhttp

github-actions bot added p2 beginning-contributor [Pilot] contributed between 0-2 PRs to the CDK labels Jan 15, 2025

aws-cdk-automation requested a review from a team January 15, 2025 01:52

aws-cdk-automation previously requested changes Jan 15, 2025

View reviewed changes

bjrara changed the title ~~feat(aws-applicationsignals-alpha): introduce Application Signals L2 constructs~~ feat(applicationsignals-alpha): introduce Application Signals L2 constructs Jan 15, 2025

bjrara force-pushed the application-signals branch 5 times, most recently from 921a486 to 43766a8 Compare January 15, 2025 04:42

aws deleted a comment from aws-cdk-automation Jan 15, 2025

bjrara force-pushed the application-signals branch 2 times, most recently from 652b1da to 021dcf9 Compare January 20, 2025 18:16

moelasmar previously requested changes Jan 21, 2025

View reviewed changes

bjrara force-pushed the application-signals branch from 021dcf9 to 6a0863c Compare January 22, 2025 04:19

bjrara force-pushed the application-signals branch 3 times, most recently from 5a2f537 to f8b8fa2 Compare January 22, 2025 07:34

github-actions bot mentioned this pull request Feb 1, 2025

Monthly PR metrics report #33266

Closed

aws-cdk-automation added the closed-for-staleness This issue was automatically closed because it hadn't received any attention in a while. label Feb 21, 2025

aws-cdk-automation closed this Feb 21, 2025

github-actions bot locked as resolved and limited conversation to collaborators Feb 21, 2025

aws unlocked this conversation Feb 23, 2025

moelasmar reopened this Feb 23, 2025

bjrara force-pushed the application-signals branch 3 times, most recently from bb1e7ce to 612770e Compare February 26, 2025 19:02

QuantumNeuralCoder removed the closed-for-staleness This issue was automatically closed because it hadn't received any attention in a while. label Feb 26, 2025

bjrara force-pushed the application-signals branch 2 times, most recently from 53bb7d4 to 54b6c75 Compare March 11, 2025 00:02

bjrara added 4 commits March 12, 2025 09:50

Add application signals enablement L2

24a3d02

Address comments

a9cd56f

Update daemon test

521a7bd

Address additional comments

d261b3e

bjrara force-pushed the application-signals branch from 54b6c75 to d261b3e Compare March 13, 2025 20:12

moelasmar self-assigned this Mar 18, 2025

Merge branch 'main' into application-signals

29d9629

moelasmar requested changes Mar 19, 2025

View reviewed changes

moelasmar reviewed Mar 19, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(applicationsignals-alpha): introduce Application Signals L2 constructs #32931

feat(applicationsignals-alpha): introduce Application Signals L2 constructs #32931

bjrara commented Jan 15, 2025

aws-cdk-automation left a comment

codecov bot commented Jan 15, 2025 •

edited

Loading

moelasmar commented Jan 16, 2025 •

edited

Loading

mergify bot commented Jan 16, 2025

moelasmar Jan 21, 2025

aws-cdk-automation commented Feb 13, 2025

aws-cdk-automation commented Feb 21, 2025

github-actions bot commented Feb 21, 2025

moelasmar commented Feb 23, 2025

bjrara commented Feb 24, 2025

bjrara commented Mar 13, 2025

aws-cdk-automation commented Mar 18, 2025

moelasmar left a comment

moelasmar Mar 19, 2025

moelasmar Mar 19, 2025

moelasmar Mar 19, 2025

moelasmar Mar 19, 2025

moelasmar Mar 19, 2025

moelasmar Mar 19, 2025

moelasmar Mar 19, 2025

feat(applicationsignals-alpha): introduce Application Signals L2 constructs #32931

Are you sure you want to change the base?

feat(applicationsignals-alpha): introduce Application Signals L2 constructs #32931

Conversation

bjrara commented Jan 15, 2025

Issue # (if applicable)

Description of changes

Describe any new or updated permissions being added

Checklist

aws-cdk-automation left a comment

Choose a reason for hiding this comment

codecov bot commented Jan 15, 2025 • edited Loading

Codecov Report

moelasmar commented Jan 16, 2025 • edited Loading

mergify bot commented Jan 16, 2025

✅ Pull request refreshed

Choose a reason for hiding this comment

aws-cdk-automation commented Feb 13, 2025

aws-cdk-automation commented Feb 21, 2025

github-actions bot commented Feb 21, 2025

moelasmar commented Feb 23, 2025

bjrara commented Feb 24, 2025

bjrara commented Mar 13, 2025

aws-cdk-automation commented Mar 18, 2025

AWS CodeBuild CI Report

moelasmar left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Jan 15, 2025 •

edited

Loading

moelasmar commented Jan 16, 2025 •

edited

Loading